Improve login layout with proper labels#1266
Improve login layout with proper labels#1266nooreldeenmansour wants to merge 3 commits intocanonical:mainfrom
Conversation
|
Thanks @nooreldeensalah
For consistency with the SSH interface, shouldn't a label also be prepended to the URL here, like so: |
dfce212 to
e1d3bb7
Compare
|
I've updated it as suggested @edibotopic, I initially followed the suggestion from this comment regarding the label becoming long on the issue. Let me know if any more changes are needed! This is how it looks like now (from |
Thanks. I think it's more visually consistent. The URL is first mentioned many lines before the QR, so it's no harm to be maximally explicit about what we are referring to, so that the connection is clear and the user can scan the text efficiently. But also, I'm not sure there is any practical advantage to centering the text for the URL and Code below the QR; if anything, it makes the text less readable. And the original comment was about the string being too long when centering is applied. So, I think this would be fine: I think that it makes more sense to have I'm not testing this, but I would also suggest (as shown in my example above) adding a newline between the QR and the strings below (if it doesn't exist already) for clearer visual separation. Any issue with the reasoning above @adombeck ? |
|
Current layout after the latest commit |
Not at all. It makes sense to me, even though I was initially a bit hesitant to replace the center alignment with left alignment, because I found it visually appealing. However, I agree that, when combined with the labels, the left alignment makes it more readable. |
pam/internal/adapter/nativemodel.go
Outdated
| @@ -731,11 +734,10 @@ func (m nativeModel) handleQrCode() tea.Cmd { | |||
| firstQrCodeLine = m.uiLayout.GetContent() | |||
There was a problem hiding this comment.
The linter complains about an ineffectual assignment to firstQrCodeLine in this line. It's correct, the variable is not used anymore, you now directly use m.uiLayout.GetContent() as the URL - which makes sense to me, because we pass the content as the URL and also use it to generate the QR code. I wonder why we tried to use the first line of the rendered QR code instead. Do you remember, @3v1n0?
| qrcodeView = append(qrcodeView, fmt.Sprintf("Code: %s", code)) | ||
| } |
There was a problem hiding this comment.
Let's add another space between the URL: label and the URL, to make it align with the code, as @edibotopic suggested in #1266 (comment):
| qrcodeView = append(qrcodeView, fmt.Sprintf("Code: %s", code)) | |
| } | |
| qrcodeView = append(qrcodeView, fmt.Sprintf("URL: %s", m.uiLayout.GetContent())) | |
| qrcodeView = append(qrcodeView, fmt.Sprintf("Code: %s", code)) | |
| } else { | |
| qrcodeView = append(qrcodeView, fmt.Sprintf("URL: %s", m.uiLayout.GetContent())) | |
| } |
That seems reasonable since this change should only affect the command-line interface. When testing changes which involve gnome-shell / GDM, you will probably want to set up a VM and share the authd repository between your host and the VM. I can help you with that. |
Yes, please! That will hopefully fix the tests which are currently failing (beside the flaky ones - if you see the "Retry Go Tests with Coverage Collection" job fail, that's a quite reliable indication that there's actually something wrong). |
18104bd to
e802ab7
Compare
Thanks, I appreciate that! it would definitely be helpful :) I've re-generated the golden tests, and pushed them in my latest commit. And I've addressed your latest suggestions as well. Current layout after adding the whitespace: SSHQR |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1266 +/- ##
==========================================
+ Coverage 86.24% 86.33% +0.08%
==========================================
Files 99 99
Lines 6690 6679 -11
Branches 111 111
==========================================
- Hits 5770 5766 -4
+ Misses 860 857 -3
+ Partials 60 56 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
e802ab7 to
125e326
Compare
|
These changes might affect our e2e-tests, so I want to see some e2e-test results before merging this. However, the e2e-tests are not run because of the issue. That will be fixed by #1384, so let's wait for that to be merged and then rebase. |
125e326 to
cbf6ed1
Compare
cbf6ed1 to
2003bc0
Compare
Closes #1263
I've tested these changes on LXD container, for the QR code testing, I've used
lxc consolecommand. I'm unsure if this is the ideal way or not.Note: I've ran the tests locally with the
TESTS_UPDATE_GOLDENflag, and it edited multiple test files, which I commited to a separate branch nooreldeenmansour@35b1378. Should I cherry-pick this commit to this PR?SSH (Before)
SSH (After)
QR (Before)
QR (After)
UDENG-9622